Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add a Grid/List toggle for Media Browser #18256

Merged
merged 4 commits into from
Nov 28, 2023

Conversation

karwosts
Copy link
Contributor

@karwosts karwosts commented Oct 16, 2023

Proposed change

Add a grid/list toggle for media browser in the top right corner overflow menu. When neither item is selected, the choice will be automatic (current behavior). Choosing one of the options sets the browser to force render in grid/list mode. Choosing it a second time reverts back to automatic behavior.

image

image

Type of change

  • Dependency upgrade
  • Bugfix (non-breaking change which fixes an issue)
  • New feature (thank you!)
  • Breaking change (fix/feature causing existing functionality to break)
  • Code quality improvements to existing code or addition of tests

Example configuration

Additional information

Checklist

  • The code change is tested and works locally.
  • There is no commented out code in this PR.
  • Tests have been added to verify that the new code works.

If user exposed functionality or configuration variables are added/changed:

@karwosts
Copy link
Contributor Author

karwosts commented Oct 16, 2023

We can also get radio list thumbnails by setting show_list_images for channel in the browser settings, though I'm not quite sure what governs the decision to add that or not.

Several other classes already have grid layout with show_list_images, so I'm not sure why some would have that combination and others not.

  album: { icon: mdiAlbum, layout: "grid" },
  app: { icon: mdiApplication, layout: "grid" },
  artist: { icon: mdiAccountMusic, layout: "grid", show_list_images: true },
  channel: {
    icon: mdiTelevisionClassic,
    thumbnail_ratio: "portrait",
    layout: "grid",
  },
  composer: {
    icon: mdiAccountMusicOutline,
    layout: "grid",
    show_list_images: true,
  },

@dunxd
Copy link

dunxd commented Oct 16, 2023

That is great - being able to read the station names is a huge step forward!

Is it possible (or easy) to add other values from the radio browser data base? An indicator of the bitrate would allow differentiation where it isn't included in the station name and there are multiple versions available.

@karwosts
Copy link
Contributor Author

I haven't actually looked at what metadata is available to the list renderer or how it works. This was a pretty simple PR as the list renderer already existed, and I just need to flip a boolean to turn it on. But I haven't actually delved into yet how it operates.

I think I will leave further enhancements to list rendering to another PR; don't have any immediate plans to personally delve into it, but maybe someday.

@dunxd
Copy link

dunxd commented Oct 16, 2023

Fair enough. This is a really helpful improvement for anyone using Radio Browser!

@frenck frenck added the Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear) label Oct 23, 2023
@bramkragten
Copy link
Member

bramkragten commented Oct 23, 2023

I like this a lot, but for the UX, I think the list should have 3 options, Auto, Grid, List.

@matthiasdebaat What do you think?

@karwosts
Copy link
Contributor Author

I like this a lot, but for the UX, I think the list should have 3 options, Auto, Grid, List.

@matthiasdebaat What do you think?

I started with this, but then I gave up trying to find an mdi icon for auto that looked good. Let me know if you have suggestion.

@bramkragten
Copy link
Member

I started with this, but then I gave up trying to find an mdi icon for auto that looked good. Let me know if you have suggestion.

Hehe, yeah not easy... Don't really have a good answer, these might work, but not the best 🤷‍♂️

Letter A for auto...
CleanShot 2023-10-23 at 15 43 48
CleanShot 2023-10-23 at 15 36 14

Combo of list and grid
CleanShot 2023-10-23 at 15 36 50

Random dashboard
CleanShot 2023-10-23 at 15 42 42

@karwosts
Copy link
Contributor Author

Added auto mode to the selection list:

image

@bramkragten
Copy link
Member

There was some discussion about this PR, regarding UX, that adding these options could cause confusion, as it is not clear what Auto does.
It might be better to solve this on the side of radio browser for example, choosing a better default view option.
But that needs some more thought probably...

@dunxd
Copy link

dunxd commented Oct 24, 2023

I agree that an Auto option really isn't clear - what decision is being made automatically? Why is this better than just having a well chosen default that can be overridden when it doesn't work for the user?

For Radio Browser the grid display leaves the feature barely useable since it isn't possible to differentiate between different options. A grid view is most useful where the most specific information is at the start of the media title, unless the image used in the grid tile shows specific information. In a well curated music collection where track numbers are at the start of the filename, grid view may work well. For Radio Browser this is definitely not the case, and users have no way of changing how Radio Browser (or thousands of individual radio stations) have decided to make entities - so it isn't something a user can resolve.

Being able to switch between grid and list view is a better compromise, and at least makes Radio Browser more useful immediately.

@ildar170975
Copy link
Contributor

Never dealt with radio...
Here my thoughts for other content (with assumptions that HA can read music files' embedded tags & HA can show thumbnails for images, music & video files):

  1. If a folder has a mixed content: "Auto" = "Grid".
  2. If a folder has images only: "Auto" = "Grid".
  3. If a folder has music files only and "Album" is different: "Auto" = "Grid".
  4. If a folder has music files only and "Album" is same: "Auto" = "List".

@bramkragten
Copy link
Member

I improved the list view a bit so it works better for all use cases instead of only the use cases it was previously used for, screenshot from the OP now looks like this:

CleanShot 2023-10-25 at 14 37 28

@karwosts
Copy link
Contributor Author

@bramkragten I'm confused what the status of this is. Your initial comment sounded like a reject, but then you pushed additional changes to the PR, so I'm not sure what we're doing with this.

@bramkragten
Copy link
Member

@bramkragten I'm confused what the status of this is. Your initial comment sounded like a reject, but then you pushed additional changes to the PR, so I'm not sure what we're doing with this.

Haha 😅 I'm going to discuss this with UX, will get back to you tomorrow.

Personally I think it is a great feature, we just need to get the UX right :-)

@bramkragten bramkragten enabled auto-merge (squash) November 28, 2023 16:26
@bramkragten bramkragten merged commit 9f1cd80 into home-assistant:dev Nov 28, 2023
9 checks passed
@karwosts karwosts deleted the media-list branch November 28, 2023 18:21
@github-actions github-actions bot locked and limited conversation to collaborators Nov 27, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
cla-signed hacktoberfest Noteworthy Marks a PR as noteworthy and should be in the release notes (in case it normally would not appear)
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants